fix(adc): calibration, efuse, and register-sequence bugs across all chips - #5589
Draft
MabezDev wants to merge 2 commits into
Draft
fix(adc): calibration, efuse, and register-sequence bugs across all chips#5589MabezDev wants to merge 2 commits into
MabezDev wants to merge 2 commits into
Conversation
Found by comparing the esp-hal ADC driver against ESP-IDF's components/esp_adc + components/hal + components/efuse. All independently contribute to wrong/garbage readings; none of them needed each other. Calibration: - C6 0 dB curve K0 coefficient was 10x too small (-0.0487... vs -0.487166...). Stale IDF revision; the upstream divisor changed to 1e15 for that one term. - S3 calibration was writing the cal attenuation to a non-existent atten slot (channel 15). The RTC controller falls back to channel 0's atten when all channels are disabled, so write there instead — matches ESP-IDF's `cal_setup`. - RISC-V calibration was passing the channel-15 sentinel through `config_onetime_sample`, which on ADC1 wrote `0x7` instead of the required `0xF` cal-mux value. Split into a dedicated `config_cal_sample`. Efuse parsing: - C2 atten-11dB cal voltage read the sign bit from the wrong variable and had inverted branches. - H2 cal_code was never sign-extended at bit 9 (~500 digi-counts of gain error when that bit was set) and skipped its version check. - H2 init_code's version check was effectively a no-op (`version > 4` with version only ever 0/1). Register sequencing: - C3 ADC2 wrote `channel` to `onetime_channel` instead of `(adc_n << 3) | channel`, so every ADC2 read sampled an ADC1 pad. Now also clears the other unit's onetime bit so alternating ADC1/ADC2 reads don't leave both units enabled. - `start_onetime_sample` had no rising edge — it called `set_bit` on a bit that was usually already set. Replaced with a proper clear -> (C6 delay) -> set, dropping the old broken double-start workaround in `read_oneshot`. - S2/S3 `read_data` returned the 16-bit field unmasked, exposing the ADC2 arbiter/validity flag bits as part of the result. - ESP32 ADC2's switch to RTC controller was missing `sar2_pwdet_force = 0` and `SYSCON.saradc_ctrl.sar2_mux = 1`; without these, after any Wi-Fi/BT init ADC2 stayed muxed to the power-detect path. Also renamed several inline bitmasks to named constants (`ADC_VAL_MASK`, `ONETIME_CHANNEL_MASK`, `ONETIME_UNIT2_BIT`, `ONETIME_CAL_CHANNEL`, `CAL_ATTEN_CHANNEL`) and added a `disable_channels` helper on xtensa to replace the `1 << 15`-mask-truncation trick.
Each DAC channel on ESP32 and ESP32-S2 lives on the same pad as one of the ADC2 channels (ESP32: GPIO25/26 -> ADC2_CH8/9; ESP32-S2: GPIO17/18 -> ADC2_CH6/7). This test uses that internal wiring as a no-rig signal source: write known DAC codes, sample the same pad with the ADC, and assert that the reading is in the right ballpark. Cases: - DAC=0 reads in the bottom 10% of full scale - DAC=255 reads in the top 20% - Midpoint (DAC=128) lands in the middle half - Full sweep is monotonic (modulo ~1.2% noise band) and spans >70% FS - Cross-channel independence: driving one DAC must not affect the other pad. Pre-fix on RISC-V the ADC2 channel encoding bug would have failed this; covers the same class of bug on Xtensa. Thresholds are all fractions of ADC_MAX because the resolution differs per chip: ESP32 returns 12-bit (default of its configurable Resolution), ESP32-S2 returns 13-bit unconditionally. Drive-by: ungate CAL_ATTEN_CHANNEL in esp-hal/src/analog/adc/xtensa.rs so the generic adc_calibrate body still name-resolves on ESP32-S2 (it has no CalibrationAccess impl, but the body is still typechecked).
Member
Author
|
Need more setup for the HIL tests to work, will iterate locally in the meantime. |
|
New commits in main have made this PR unmergeable. Please resolve the conflicts. |
github-actions
Bot
added
the
merge-conflict
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Side-by-side diff of
esp-hal's ADC against ESP-IDF (components/esp_adc,components/hal,components/efuse). Each item is independently contributing to wrong/garbage readings; none of them depend on each other.Calibration
config_onetime_sample, which on ADC1 wrote0x7instead of the required0xFcal-mux value. Split into a dedicatedconfig_cal_sampleEfuse parsing
cal_codenever sign-extended at bit 9 (~500 digi-counts of gain error when set); also missing version checks oncal_codeandinit_codeRegister sequencing
channeltoonetime_channelinstead of(adc_n << 3) | channel— every ADC2 read sampled an ADC1 pad. Also clears the other unit's onetime bitstart_onetime_samplehad no rising edge; replaced withclear -> (C6 delay) -> set, drops the old broken double-start workaroundread_datareturned the 16-bit field unmasked, exposing arbiter/validity flag bitssar2_pwdet_force = 0andSYSCON.saradc_ctrl.sar2_mux = 1; without these, after Wi-Fi/BT init ADC2 stayed muxed to the power-detect pathHIL test
DAC→ADC internal-loopback test on
esp32/esp32s2(the only chips with both peripherals). Each DAC channel shares its pad with an ADC2 channel, so no wiring needed. Verifies zero/max/midpoint/sweep/cross-channel independence. Thresholds expressed as fractions ofADC_MAXso the same test scales between ESP32's 12-bit and S2's 13-bit output.Future work
Deliberately out of scope here; tracked for follow-ups:
meas_statusbusy-wait before SAR1 start on ESP32 / S2 / S3 — race that can return stale data on tight read loops.